TEL-886: Media Port rework - #802
Conversation
11f4b21 to
dd06f29
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Huh, the race in CI is real, and is apparently a problem in media-sdk (yay zerocopy) that previous tests simply did not expose. |
Will fix this in media-sdk. |
6a063e6 to
dce2728
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| w := c.lkRoom.SwapOutput(nil) | ||
| // Mute the room audio to the SIP participant. | ||
| // Skip closing the existing writer, which is c.audioOut. | ||
| _ = c.lkRoom.WriteOutboundAudioTo(nil) |
There was a problem hiding this comment.
Why not keep w as it was before? It makes sure we swap back to the same pipeline that was used before the transfer.
For closing, if I read it correctly, it's already protected by NopCloser in the media pipeline, right? So we should close here to avoid leaks, and media pipeline will decide if it wants to expose close or not.
| return errors.New("unexpected local address change") | ||
| } | ||
|
|
||
| audioToPort := p.audioOut.Swap(nil) // either nil or no-op closer |
There was a problem hiding this comment.
Maybe assert that it's the case during swap?
There was a problem hiding this comment.
Not sure I follow. Are you talking about something like:
audioToPort := p.audioOut.Swap(nil)
if audioToPort != nil {
if _, ok := audioToPort.(msdk.writeCloser[x]); !ok {
log.Errorw(...)
}
}
?
The only thin that swaps these is the configure function itself, so I'm not sure what the benefit would be there.
The comment is there to explain why we don't close the popped value.
|
|
||
| // WriteOutboundDTMFTo tells the room where to send DTMF to. | ||
| // Returns the previously-set writer (if one exists). | ||
| WriteOutboundDTMFTo(w msdk.WriteCloser[*livekit.SipDTMF]) msdk.WriteCloser[*livekit.SipDTMF] |
There was a problem hiding this comment.
Any specific reason to use pointers in livekit.SipDTMF? It's a tiny payload, might as well just copy it around.
There was a problem hiding this comment.
I guess the thread safety is a good benefit of copying. Fair enough.
There was a problem hiding this comment.
WriteSample passes lock by value: github.com/livekit/protocol/livekit.SipDTMF contains google.golang.org/protobuf/runtime/protoimpl.MessageState contains sync.Mutex I guess this is one,
df94a89 to
4ab15fe
Compare
47b61bd to
5ad9b22
Compare
3 participants
This change is intended to: